Three tests failed, two of which had been failing before and one which is definitely related:

------- Stdout: -------
=== RUN   TestAccContainerCluster_sharedVpc
--- FAIL: TestAccContainerCluster_sharedVpc (274.04s)
	testing.go:518: Step 0 error: Error applying: 1 error(s) occurred:
		
		* google_container_cluster.shared_vpc_cluster: 1 error(s) occurred:
		
		* google_container_cluster.shared_vpc_cluster: googleapi: Error 400: Using a shared network is only supported with IP aliases., badRequest
FAIL

Investigating a bit because I'd like this change to go in fairly soon. So here's the request that's getting sent:

{"cluster":{"binaryAuthorization":{"enabled":false},"initialNodeCount":1,"ipAllocationPolicy":{"clusterSecondaryRangeName":"pods","servicesSecondaryRangeName":"services"},"legacyAbac":{"enabled":false},"name":"cluster-test-vi2jya6lx9","network":"projects/tf-xpntest-9x7wdsi3uc-host/global/networks/test-eggvh0tbko","nodeConfig":{"oauthScopes":["https://www.googleapis.com/auth/devstorage.read_only","https://www.googleapis.com/auth/logging.write","https://www.googleapis.com/auth/monitoring","https://www.googleapis.com/auth/service.management.readonly","https://www.googleapis.com/auth/servicecontrol","https://www.googleapis.com/auth/trace.append"]},"subnetwork":"projects/tf-xpntest-9x7wdsi3uc-host/regions/us-central1/subnetworks/test-7ltede4c6s"}}

It definitely isn't setting ipAliases to true, which would make sense because it's not in the config. However, this PR didn't actually remove the line where we always set it to true: https://github.com/terraform-providers/terraform-provider-google/blob/6bb3fdedf08762e9bb3f37489debfa0ac991de9e/google/resource_container_cluster.go#L1428 so I have no idea why it isn't being set to true. Either way, this is going to need a little extra work because we were previously always setting it as true- we might want to make the default true, or something else so we don't accidentally break people.

Ok cool, so the reason it didn't set useIpAliases to true is just because we set it to false later. Cool.

What I think we actually want to do here is leave useIpAliases alone, but add the other fields in ipAllocationPolicy.

Paddy, requesting your review since I made a bunch of changes since Nathan's original PR.

One of the things I did was changing our existing IP Aliases test to be a bit simpler- we no longer check the failure cases. One of them I removed because we removed the error, and the other I removed because it's testing that the GKE API returns a specific error when you give it bad input, which I don't think is what we really should be aiming to test in Terraform (I want to test terraform's behavior, not GKE's). Let me know if that sounds right to you too!

@danawillow FWIW, your changes LGTM, so if the final PR LGTYou, I think we can submit it.

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!